-
-
Notifications
You must be signed in to change notification settings - Fork 824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[php8-compact][REF] Fix failing custom group test on php8 by better h… #20616
[php8-compact][REF] Fix failing custom group test on php8 by better h… #20616
Conversation
(Standard links)
|
a5ca284
to
1a8057c
Compare
This needs #20617 to be merged first to be able to pass |
11c7517
to
ee185f7
Compare
$registeredSubTypes = self::getSubTypes()[$params['extends']]; | ||
if (is_array($extendsChildType)) { | ||
foreach ($extendsChildType as $childType) { | ||
if (!array_key_exists($childType, $registeredSubTypes) && !in_array($childType, $registeredSubTypes, TRUE)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug in this validation. If you try to create a ParticipantRole group with a type through the form it fails because registeredTypes above has a key 'ParticipantRole' but $params['extends'] is 'Participant'
I tested this & it seems to work
--- a/CRM/Core/BAO/CustomGroup.php
+++ b/CRM/Core/BAO/CustomGroup.php
@@ -51,7 +51,9 @@ class CRM_Core_BAO_CustomGroup extends CRM_Core_DAO_CustomGroup {
$extendsChildType = $params['extends_entity_column_value'];
}
if (!CRM_Utils_System::isNull($extendsChildType)) {
- $registeredSubTypes = self::getSubTypes()[$params['extends']];
+
+ $b = self::getMungedEntity($params['extends'], $params['extends_entity_column_id'] ?? NULL);
+ $registeredSubTypes = self::getSubTypes()[$b];
if (is_array($extendsChildType)) {
foreach ($extendsChildType as $childType) {
if (!array_key_exists($childType, $registeredSubTypes) && !in_array($childType, $registeredSubTypes, TRUE)) {
@@ -224,6 +226,24 @@ class CRM_Core_BAO_CustomGroup extends CRM_Core_DAO_CustomGroup {
return CRM_Core_DAO::commonRetrieve('CRM_Core_DAO_CustomGroup', $params, $defaults);
}
+ /**
+ * Get the munged entity.
+ *
+ * This is the entity eg. Relationship or the name of the sub entity
+ * e.g ParticipantRole.
+ *
+ * @param string $extends
+ * @param int|null $extendsEntityColumn
+ *
+ * @return string
+ */
+ protected static function getMungedEntity($extends, $extendsEntityColumn = NULL) {
+ if (!$extendsEntityColumn) {
+ return $extends;
+ }
+ return CRM_Core_OptionGroup::values('custom_data_type', FALSE, FALSE, FALSE, NULL, 'name')[$extendsEntityColumn];
+ }
I hit a bug in r-run. I'm happy with the code once it passes r-run but want to note that it seems possible to change types (at least before fields are added) so I think r-run needs to include that & ensuring it's possible to change from a subtype-kinda-one to a main type & the reverse & the db columns are correctly updated |
…andling strings in 2nd key of the extends array and also validating the child and main entity work
…te into apiv3 and change Custom Group form to use apiv3
… type and add unit tests to lock in fix
ee185f7
to
92ef782
Compare
thanks for the review @eileenmcnaughton I have applied your fix with a small modification and have also added 2 unit tests 1 covering the bug that you hit and 1 covering the changing of the entity when the custom group doesn't have any fields |
I did another r-run and was able to save ParticipantRole and change between types (including sub & non-sub types) |
…andling strings in 2nd key of the extends array and also validating the child and main entity work
Overview
This fixes a failing unit test in CustomGroup API testing that is because we pass a string rather than an array as a 2nd param to the extends in API and it was somewhat not well handled. It does it by adding in validation on the subtype + entity combination
Before
testCustomGroupExtendsMultipleCreate test fails because php8 is harder on variable types in implode
After
testCustomGroupExtendsMultipleCreate passes on php8
ping @eileenmcnaughton @colemanw @totten @demeritcowboy